Skip to content

Conversation

@romanc
Copy link
Collaborator

@romanc romanc commented Aug 13, 2025

Description

As mentioned in #186 (comment), our caching is currently sub-optimal and only really works for subsequent runs on the same PR.

Ideally, we'd like to share caches between PRs such that not every PR has to first build a cache. The way GHA caches work is that they can only be accessed if they were created by the same PR or by the target branch. I've thus added a workflow to (re-)create the pre-commit cache whenever we merge into the develop branch. The linting workflow (running on PRs) will just restore the cache (and not attempt to upload a new one).

Caching the test_data needs to be done in the workflow that creates the cache (e.g., for the "PyFV3 translate tests" we need to modify the workflow(s) in the PyFV3 repo).

We could cache the pip cache for "NDSL unit tests". However, since we (mostly) aren't pinning python package dependencies to specific version numbers (in setup.py and pyproject.toml), this cache would get out of date soon. And since the runtime of NDSL unit tests are governed by the test runtime, I don't see a need for that right now. Once we pin package dependencies with a lock file, we can re-evaluate.

The changes in the pull request template are unrelated. They just stop my editor from complaining about invalid markdown syntax in that file. The changes are due to the following rules

  • every markdown starts with a # Header
  • don't use **bold font** as headers
  • headers are followed by an empty line

How Has This Been Tested?

Tested the new workflow on my fork of NDSL.

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation: N/A
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules: N/A
  • New check tests, if applicable, are included: N/A

@romanc romanc requested a review from fmalatino August 13, 2025 10:25
@romanc
Copy link
Collaborator Author

romanc commented Aug 20, 2025

@fmalatino any comments on this PR as a proof of concept? Do you think it's legit or totally over-engineered?

@fmalatino
Copy link
Contributor

It looks good! And I agree, once we move to pinned versions we should investigate doing this for the unit tests as well.

@romanc
Copy link
Collaborator Author

romanc commented Sep 3, 2025

Revisiting this old proof of concept since we have a little room again. As mentioned, there's not a whole lot to cache at the moment (since we don't lock python dependencies), but I'd like to gather some experience with this technique to see how it holds up in usage compared to on paper.

@romanc
Copy link
Collaborator Author

romanc commented Sep 5, 2025

Any more thoughts on this? I'm fine either way, just want to get to a resolution soon.

Copy link
Collaborator

@FlorianDeconinck FlorianDeconinck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yez pleaz

@romanc romanc added this pull request to the merge queue Sep 5, 2025
Merged via the queue into NOAA-GFDL:develop with commit 0ead2ed Sep 5, 2025
5 checks passed
@romanc romanc deleted the romanc/create-cache branch September 8, 2025 08:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants